-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Suppress errors from well-formed-testing type traits in SFINAE contexts #135390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Suppress errors from well-formed-testing type traits in SFINAE contexts #135390
Conversation
|
@llvm/pr-subscribers-clang Author: Aaron Puchert (aaronpuchert) ChangesThere are several type traits that produce a boolean value or type based on the well-formedness of some expression (more precisely, the immediate context, i.e. for example excluding nested template instantiation):
Errors in the immediate context are suppressed, instead the type traits return another value or produce a different type if the expression is not well-formed. This is achieved using an So what we want to do is always set Going through uses of Because the parameter no longer just sets The added tests should more or less correspond to the users of We put the Fixes #132044. Full diff: https://github.com/llvm/llvm-project/pull/135390.diff 6 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f65dd0191c666..971339b84f08a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -12240,16 +12240,16 @@ class Sema final : public SemaBase {
bool PrevLastDiagnosticIgnored;
public:
- explicit SFINAETrap(Sema &SemaRef, bool AccessCheckingSFINAE = false)
+ explicit SFINAETrap(Sema &SemaRef, bool TestWellformedSFINAE = false)
: SemaRef(SemaRef), PrevSFINAEErrors(SemaRef.NumSFINAEErrors),
PrevInNonInstantiationSFINAEContext(
SemaRef.InNonInstantiationSFINAEContext),
PrevAccessCheckingSFINAE(SemaRef.AccessCheckingSFINAE),
PrevLastDiagnosticIgnored(
SemaRef.getDiagnostics().isLastDiagnosticIgnored()) {
- if (!SemaRef.isSFINAEContext())
+ if (TestWellformedSFINAE || !SemaRef.isSFINAEContext())
SemaRef.InNonInstantiationSFINAEContext = true;
- SemaRef.AccessCheckingSFINAE = AccessCheckingSFINAE;
+ SemaRef.AccessCheckingSFINAE = TestWellformedSFINAE;
}
~SFINAETrap() {
@@ -12279,7 +12279,7 @@ class Sema final : public SemaBase {
public:
explicit TentativeAnalysisScope(Sema &SemaRef)
- : SemaRef(SemaRef), Trap(SemaRef, true),
+ : SemaRef(SemaRef), Trap(SemaRef, /*TestWellformedSFINAE=*/true),
PrevDisableTypoCorrection(SemaRef.DisableTypoCorrection) {
SemaRef.DisableTypoCorrection = true;
}
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 011a6d072d35c..8519ff472d5a0 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -908,7 +908,7 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
if (MLTAL.getNumSubstitutedLevels() == 0)
return ConstrExpr;
- Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+ Sema::SFINAETrap SFINAE(S);
Sema::InstantiatingTemplate Inst(
S, DeclInfo.getLocation(),
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 247cd02b23522..578d441cf0876 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5198,7 +5198,7 @@ static bool HasNonDeletedDefaultedEqualityComparison(Sema &S,
{
EnterExpressionEvaluationContext UnevaluatedContext(
S, Sema::ExpressionEvaluationContext::Unevaluated);
- Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+ Sema::SFINAETrap SFINAE(S, /*TestWellformedSFINAE=*/true);
Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
// const ClassT& obj;
@@ -5809,7 +5809,7 @@ static ExprResult CheckConvertibilityForTypeTraits(
// trap at translation unit scope.
EnterExpressionEvaluationContext Unevaluated(
Self, Sema::ExpressionEvaluationContext::Unevaluated);
- Sema::SFINAETrap SFINAE(Self, /*AccessCheckingSFINAE=*/true);
+ Sema::SFINAETrap SFINAE(Self, /*TestWellformedSFINAE=*/true);
Sema::ContextRAII TUContext(Self, Self.Context.getTranslationUnitDecl());
InitializationSequence Init(Self, To, Kind, From);
if (Init.Failed())
@@ -5932,7 +5932,7 @@ static bool EvaluateBooleanTypeTrait(Sema &S, TypeTrait Kind,
// trap at translation unit scope.
EnterExpressionEvaluationContext Unevaluated(
S, Sema::ExpressionEvaluationContext::Unevaluated);
- Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+ Sema::SFINAETrap SFINAE(S, /*TestWellformedSFINAE=*/true);
Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
InitializedEntity To(
InitializedEntity::InitializeTemporary(S.Context, Args[0]));
@@ -6272,7 +6272,7 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
// trap at translation unit scope.
EnterExpressionEvaluationContext Unevaluated(
Self, Sema::ExpressionEvaluationContext::Unevaluated);
- Sema::SFINAETrap SFINAE(Self, /*AccessCheckingSFINAE=*/true);
+ Sema::SFINAETrap SFINAE(Self, /*TestWellformedSFINAE=*/true);
Sema::ContextRAII TUContext(Self, Self.Context.getTranslationUnitDecl());
ExprResult Result = Self.BuildBinOp(/*S=*/nullptr, KeyLoc, BO_Assign, &Lhs,
&Rhs);
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 40e29bb18807a..d7408fdd4f560 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -3112,7 +3112,7 @@ static QualType builtinCommonTypeImpl(Sema &S, TemplateName BaseTemplate,
EnterExpressionEvaluationContext UnevaluatedContext(
S, Sema::ExpressionEvaluationContext::Unevaluated);
- Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+ Sema::SFINAETrap SFINAE(S, /*TestWellformedSFINAE=*/true);
Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
QualType BaseTemplateInst =
@@ -3158,7 +3158,7 @@ static QualType builtinCommonTypeImpl(Sema &S, TemplateName BaseTemplate,
auto CheckConditionalOperands = [&](bool ConstRefQual) -> QualType {
EnterExpressionEvaluationContext UnevaluatedContext(
S, Sema::ExpressionEvaluationContext::Unevaluated);
- Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+ Sema::SFINAETrap SFINAE(S, /*TestWellformedSFINAE=*/true);
Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
// false
diff --git a/clang/test/SemaCXX/type-trait-common-type.cpp b/clang/test/SemaCXX/type-trait-common-type.cpp
index 7190dcad76f1a..f21a5a1fadbc1 100644
--- a/clang/test/SemaCXX/type-trait-common-type.cpp
+++ b/clang/test/SemaCXX/type-trait-common-type.cpp
@@ -34,6 +34,7 @@ void test_vla() {
template <class... Args>
using common_type_base = __builtin_common_type<common_type_t, type_identity, empty_type, Args...>;
+// expected-note@-1 {{in instantiation of default function argument expression for 'InvalidConversion<void>' required here}}
template <class... Args>
struct common_type : common_type_base<Args...> {};
@@ -208,3 +209,36 @@ struct common_type<PrivateTypeMember, PrivateTypeMember>
};
static_assert(__is_same(common_type_base<PrivateTypeMember, PrivateTypeMember, PrivateTypeMember>, empty_type));
+
+class PrivateConstructor {
+private:
+ PrivateConstructor(int);
+};
+
+static_assert(__is_same(common_type_base<int, PrivateConstructor>, empty_type));
+
+// expected-note@+1 {{in instantiation of template type alias 'common_type_base' requested here}}
+template<typename A, typename B, typename Res = common_type_base<A, B>>
+static Res common_type_sfinae();
+// expected-note@-1 {{in instantiation of default argument for 'common_type_sfinae<int, InvalidConversion>' required here}}
+
+// Make sure we swallow "calling a private constructor" even in SFINAE context ...
+static_assert(__is_same(decltype(common_type_sfinae<int, PrivateConstructor>()), empty_type));
+
+// ... but not when there is an error outside of the immediate context.
+template<typename T>
+struct Member {
+ T t; // expected-error {{field has incomplete type 'void'}}
+};
+
+// The conversion from int has a non-SFINAE error.
+class InvalidConversion {
+private:
+ template<typename T = void>
+ InvalidConversion(int, Member<T> = {});
+ // expected-note@-1 {{in instantiation of template class 'Member<void>' requested here}}
+ // expected-note@-2 {{passing argument to parameter here}}
+};
+
+// expected-note@+1 {{while substituting deduced template arguments into function template 'common_type_sfinae'}}
+static_assert(__is_same(decltype(common_type_sfinae<int, InvalidConversion>()), empty_type));
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index b130024503101..51986fcecd83d 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -2673,6 +2673,9 @@ struct FloatWrapper
}
};
+template<typename A, typename B, bool = __is_convertible(A, B)>
+static void is_convertible_sfinae();
+
void is_convertible()
{
static_assert(__is_convertible(IntWrapper, IntWrapper));
@@ -2697,6 +2700,10 @@ void is_convertible()
static_assert(__is_convertible(FloatWrapper, const float&));
static_assert(__is_convertible(float, FloatWrapper&&));
static_assert(__is_convertible(float, const FloatWrapper&));
+
+ static_assert(!__is_convertible(AllPrivate, AllPrivate));
+ // Make sure we swallow "calling a private constructor" even in SFINAE context.
+ using Result = decltype(is_convertible_sfinae<AllPrivate, AllPrivate>());
}
void is_nothrow_convertible()
@@ -2822,6 +2829,9 @@ void is_trivial()
template<typename T> struct TriviallyConstructibleTemplate {};
+template<typename A, typename B, bool = __is_assignable(A, B)>
+static void is_assignable_sfinae();
+
void trivial_checks()
{
static_assert(__is_trivially_copyable(int));
@@ -2995,6 +3005,10 @@ void trivial_checks()
static_assert(!__is_assignable(AnIncompleteType[1], AnIncompleteType[1])); // expected-error {{incomplete type}}
static_assert(!__is_assignable(void, void));
static_assert(!__is_assignable(const volatile void, const volatile void));
+
+ static_assert(!__is_assignable(AllPrivate, AllPrivate));
+ // Make sure we swallow "'operator=' is a private member" even in SFINAE context.
+ using Result = decltype(is_assignable_sfinae<AllPrivate, AllPrivate>());
}
void constructible_checks() {
@@ -3191,6 +3205,9 @@ void reference_constructs_from_temporary_checks() {
}
+template<typename A, typename B, bool = __reference_converts_from_temporary(A, B)>
+static void reference_converts_from_temporary_sfinae();
+
void reference_converts_from_temporary_checks() {
static_assert(!__reference_converts_from_temporary(int &, int &));
static_assert(!__reference_converts_from_temporary(int &, int &&));
@@ -3241,6 +3258,9 @@ void reference_converts_from_temporary_checks() {
static_assert(__reference_converts_from_temporary(const int&, ExplicitConversionRef));
static_assert(__reference_converts_from_temporary(int&&, ExplicitConversionRvalueRef));
+ static_assert(!__reference_converts_from_temporary(AllPrivate, AllPrivate));
+ // Make sure we swallow "calling a private constructor" even in SFINAE context.
+ using Result = decltype(reference_converts_from_temporary_sfinae<AllPrivate, AllPrivate>());
}
void array_rank() {
@@ -4075,6 +4095,20 @@ struct NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2 {
static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2));
+struct NotTriviallyEqualityComparablePrivateComparison {
+ int i;
+
+private:
+ bool operator==(const NotTriviallyEqualityComparablePrivateComparison&) const = default;
+};
+static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparablePrivateComparison));
+
+template<typename T, bool = __is_trivially_equality_comparable(T)>
+static void is_trivially_equality_comparable_sfinae();
+
+// Make sure we swallow "'operator==' is a private member" even in SFINAE context.
+using SFINAEResult = decltype(is_trivially_equality_comparable_sfinae<NotTriviallyEqualityComparablePrivateComparison>());
+
template<bool B>
struct MaybeTriviallyEqualityComparable {
int i;
|
|
The tests would produce without the change: and |
5c6ff2e to
a1eda3b
Compare
a1eda3b to
2021935
Compare
Filed cplusplus/draft#7827 to clarify this. |
ef1b40c to
af21e7b
Compare
cor3ntin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a changelog entry?
Other than that, the change makes sense to me
af21e7b to
d8bc5eb
Compare
|
I would push the change log entry separately to avoid conflicts if you don't mind. Here is what I would add (under "Bug Fixes in This Version"):
|
|
Could also add this under "Bug Fixes to C++ Support". The entries about type traits don't seem to be consistently categorized, but I think this is all C++-only. |
|
@aaronpuchert Bug Fixes to C++ Support would be the correct place, yes. You can put the entry somewhere other than the end of the list to minimize the risk of conflicts, but it should be in the same commit. |
cor3ntin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo missing release note
There are several type traits that produce a boolean value or type based
on the well-formedness of some expression (more precisely, the immediate
context, i.e. for example excluding nested template instantiation):
* `__is_constructible` and variants,
* `__is_convertible` and variants,
* `__is_assignable` and variants,
* `__reference_{binds_to,{constructs,converts}_from}_temporary`,
* `__is_trivially_equality_comparable`,
* `__builtin_common_type`.
(It should be noted that the standard doesn't always base this on the
immediate context being well-formed: for `std::common_type` it's based
on whether some expression "denotes a valid type." But I assume that's
an editorial issue and means the same thing.)
Errors in the immediate context are suppressed, instead the type traits
return another value or produce a different type if the expression is
not well-formed. This is achieved using an `SFINAETrap` with
`AccessCheckingSFINAE` set to true. If the type trait is used outside of
an SFINAE context, errors are discarded because in that case the
`SFINAETrap` sets `InNonInstantiationSFINAEContext`, which makes
`isSFINAEContext` return an `optional(nullptr)`, which causes the errors
to be discarded in `EmitDiagnostic`. However, in an SFINAE context this
doesn't happen, and errors are added to `SuppressedDiagnostics` in the
`TemplateDeductionInfo` returned by `isSFINAEContext`. Once we're done
with deducing template arguments and have decided which template is
going to be instantiated, the errors corresponding to the chosen
template are then emitted. At this point we get errors from those type
traits that we wouldn't have seen if used with the same arguments
outside of an SFINAE context. That doesn't seem right.
So what we want to do is always set `InNonInstantiationSFINAEContext`
when evaluating these well-formed-testing type traits, regardless of
whether we're in an SFINAE context or not. This should only affect the
immediate context, as nested contexts add a new `CodeSynthesisContext`
that resets `InNonInstantiationSFINAEContext` for the time it's active.
Going through uses of `SFINAETrap` with `AccessCheckingSFINAE` = `true`,
it occurred to me that all of them want this behavior and we can just
use this parameter to decide whether to use a non-instantiation context.
The uses are precisely the type traits mentioned above plus the
`TentativeAnalysisScope`, where I think it is also fine. (Though I think
we don't do tentative analysis in SFINAE contexts anyway.)
Because the parameter no longer just sets `AccessCheckingSFINAE` in Sema
but also `InNonInstantiationSFINAEContext`, I think it should be renamed
(along with uses, which also point the reviewer to the affected places).
Since we're testing for validity of some expression, `ForValidityCheck`
seems to be a good name.
The added tests should more or less correspond to the users of
`SFINAETrap` with `AccessCheckingSFINAE` = `true`. I added a test for
errors outside of the immediate context for only one type trait, because
it requires some setup and is relatively noisy.
We put the `ForValidityCheck` condition first because it's constant in
all uses and this would then allow the compiler to prune the call to
`isSFINAEContext` when true.
Fixes llvm#132044.
d8bc5eb to
7d0a3fb
Compare
|
Ok, added it close to another note that seemed mildly related. |
There are several type traits that produce a boolean value or type based on the well-formedness of some expression (more precisely, the immediate context, i.e. for example excluding nested template instantiation):
__is_constructibleand variants,__is_convertibleand variants,__is_assignableand variants,__reference_{binds_to,{constructs,converts}_from}_temporary,__is_trivially_equality_comparable,__builtin_common_type.(It should be noted that the standard doesn't always base this on the immediate context being well-formed: for
std::common_typeit's based on whether some expression "denotes a valid type." But I assume that's an editorial issue and means the same thing.)Errors in the immediate context are suppressed, instead the type traits return another value or produce a different type if the expression is not well-formed. This is achieved using an
SFINAETrapwithAccessCheckingSFINAEset to true. If the type trait is used outside of an SFINAE context, errors are discarded because in that case theSFINAETrapsetsInNonInstantiationSFINAEContext, which makesisSFINAEContextreturn anoptional(nullptr), which causes the errors to be discarded inEmitDiagnostic. However, in an SFINAE context this doesn't happen, and errors are added toSuppressedDiagnosticsin theTemplateDeductionInforeturned byisSFINAEContext. Once we're done with deducing template arguments and have decided which template is going to be instantiated, the errors corresponding to the chosen template are then emitted. At this point we get errors from those type traits that we wouldn't have seen if used with the same arguments outside of an SFINAE context. That doesn't seem right.So what we want to do is always set
InNonInstantiationSFINAEContextwhen evaluating these well-formed-testing type traits, regardless of whether we're in an SFINAE context or not. This should only affect the immediate context, as nested contexts add a newCodeSynthesisContextthat resetsInNonInstantiationSFINAEContextfor the time it's active.Going through uses of
SFINAETrapwithAccessCheckingSFINAE=true, it occurred to me that all of them want this behavior and we can just use this parameter to decide whether to use a non-instantiation context. The uses are precisely the type traits mentioned above plus theTentativeAnalysisScope, where I think it is also fine. (Though I think we don't do tentative analysis in SFINAE contexts anyway.)Because the parameter no longer just sets
AccessCheckingSFINAEin Sema but alsoInNonInstantiationSFINAEContext, I think it should be renamed (along with uses, which also point the reviewer to the affected places). Since we're testing for validity of some expression,ForValidityCheckseems to be a good name.The added tests should more or less correspond to the users of
SFINAETrapwithAccessCheckingSFINAE=true. I added a test for errors outside of the immediate context for only one type trait, because it requires some setup and is relatively noisy.We put the
ForValidityCheckcondition first because it's constant in all uses and this would then allow the compiler to prune the call toisSFINAEContextwhen true.Fixes #132044.